test: one source fingerprint, in Python, called by both harnesses (#907) - #911
Conversation
de432fa to
7f55857
Compare
|
Rebased onto One conflict, on Recounted from the corpus with the gate's own function rather than resolved by Re-gated after the rebase rather than before: The last line is the one worth reading twice: those two numbers were never equal |
…mmandprompt#907) test/lib.sh and test/pytest/pgc_cluster.py each carried their own answer to "what was this binary built from". On 2026-09-09 the pair produced four defects between them -- two in each copy, and NOT ONE was found by whoever wrote that copy: objstore/*.c never walked python @linuxhikerpm, commandprompt#897 the bare NAME instead of the path python found while fixing the above `xargs -0 cat | md5sum`, no bounds shell @linuxhikerpm, commandprompt#898 each build dir's Makefile omitted python found while writing the twin The Python docstring asserted "the same input set as pgc_source_fingerprint in test/lib.sh" throughout all four. It was false when written and stayed false through two rounds of fixing. A prose claim of agreement is not a mechanism, and it is worse than silence because it is what stops the next person checking. Python, not shell, which is the opposite of what commandprompt#907 first proposed -------------------------------------------------------------------- jd's constraint decided it: the single implementation belongs in the more portable language. bash is largely a GNU thing; Python is present on FreeBSD and Windows where bash is not. lib.sh already requires bash, so calling a more portable interpreter from it cannot cost portability. My argument against this direction was that lib.sh invokes python3 zero times, so this escalates from "53 suites need it" to "every suite needs it at gate time". That is true and it is not a cost, for the reason above. Measured, expecting to report a subprocess penalty: shell, forking md5sum once per file 239 ms/call the module, one interpreter start 26 ms/call across 261 suites x 2 fingerprints 124 s -> 13 s The portable direction is also 9x faster. I had it backwards in both dimensions. A fifth defect, which unifying them found ------------------------------------------ `sort -z` orders by LOCALE COLLATION, and nothing in this harness pins a locale. The same tree fingerprinted two ways depending on whose machine it was: LC_ALL=C 6d122a7158d5 LC_ALL=en_US.UTF-8 0b59bd75fa4f en_US.UTF-8 is a common desktop default, so this is a developer stamping a tree and CI reading it back and calling the binary stale -- a false FATAL arriving from the environment rather than from the source. The module sorts BYTES, which is what LC_ALL=C produced and what every stamp already on disk was written with, so no existing stamp is invalidated. Arms in both harnesses. Equivalence, established rather than asserted ---------------------------------------------- A differential run of the module against the shell it replaces, over trees built to break the ways this pair has actually broken. 17 shapes, manifest AND fingerprint compared: the real source tree, minimal, a recursed module, a dir with sources but no Makefile, collation-sensitive names, a symlinked source file, a symlinked build directory, no src/, an empty tree, root .control and .sql, non-source files, spaces and punctuation, unicode, a Makefile at depth 3, a trailing slash, a /./ segment, five recursed modules AGREE=17 DIVERGE=0 Two of those are subtle enough to be worth naming. `find -type f` tests the LINK, so a symlinked source is not in the shell's manifest, while `pathlib.is_file()` FOLLOWS it and would have added one; the module excludes symlinks explicitly. And `find` does not descend a symlinked directory, so build dirs discovered through one differ -- which is why the module canonicalises the root first. The mechanism of two arms had to change with the implementation ---------------------------------------------------------------- The failed-digest arms in 340 and test_build_refusal.py stubbed `md5sum` on PATH. The digest is hashlib now, which no PATH can reach, so the stub would have left both arms GREEN while testing nothing -- the exact shape this corpus refuses. A real read failure needs a real reader who is denied, and root is denied nothing: chmod 000 is invisible to it. Measured before the arms were rewritten: as root 28a7149e07ae <- reads the mode-000 file regardless as postgres (empty) <- the failure the arm needs So the tree is built outside any mode-0700 directory and read by a second user, with a premise asserting that reader agrees with a privileged one WHILE nothing is denied -- otherwise the arm measures the user switch rather than the failure. Where no non-root user exists it records expect.cannot_run rather than passing. And the arm that would catch this issue recurring -------------------------------------------------- selftest 380's static guards follow the fingerprint to its new file, plus three new arms: neither caller may keep a private implementation, and the module may import nothing from test/pytest/. A static assertion of ABSENCE is the shape that most often cannot fail, so each was proved against the REAL files rather than only against fixtures -- a fixture proves the pattern matches something, not that the arm aimed at the real file would fire: pgc_cluster.py grows a private digest HELD lib.sh grows a private md5sum loop HELD the module imports from the pytest tree HELD harness_selftest 407 passed + 0 failed + 0 unrunnable, rc=0 pytest corpus 91 passed docs_style 9 checks PASSED shellcheck -S error clean Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
7f55857 to
b71b9fc
Compare
|
Rebased onto Two conflicts this time. The Recounted from the corpus, not picked. The prose count moved with it to That value moved from |
jdatcmd
left a comment
There was a problem hiding this comment.
Approving at b71b9fc1. This replaces the implementation I landed six hours ago, so I verified the properties #909 established rather than trusting that a port preserves them — and one measurement of mine was wrong in a way that nearly cost you a false blocker.
The property that mattered most: stamps on disk stay valid
A port that changes the hash makes every stamp already written read as stale — a false FATAL on every developer's tree, invisible to CI because CI copies a fresh tree and re-stamps. Same tree, both implementations:
old shell implementation (a9c40b1) : 213b0077e930
new python port (b71b9fc) : 213b0077e930 IDENTICAL
And on a tree built to be awkward — a symlinked FILE, a symlinked DIRECTORY, a .c in a directory with no Makefile, every hashed suffix at the root:
old : b133dc404e36
new : b133dc404e36 IDENTICAL
The symlink cases are the ones I would have got wrong. find -type f tests the LINK, so a symlinked source is not in the shell's manifest; pathlib.is_file() FOLLOWS it and would have silently added one. The module excludes them explicitly and the hashes agree.
The fifth defect is real on THIS tree, not a hypothetical
I checked, because a defect demonstrated only on a constructed filename pair might never bite here:
OLD implementation, the real tree
LC_ALL=C 6d122a7158d5
LC_ALL=C.UTF-8 6d122a7158d5
LC_ALL=en_US.UTF-8 0b59bd75fa4f <- same tree, different answer
NEW port, same three locales
213b0077e930 / 213b0077e930 / 213b0077e930
Exactly the two values you quoted. And the tree really does carry the sensitive shape — columnar.h against columnar_arrow.c, where . and _ collate differently:
C order columnar-arrow.c columnarZarrow.c columnar_arrow.c
en_US order columnar_arrow.c columnar-arrow.c columnarZarrow.c
A developer on an en_US desktop stamping a tree that CI then reads under C.UTF-8 is a FATAL naming a stale binary against a clean tree. Sorting bytes removes the environment from the answer.
#909's guarantees, each re-checked
empty tree -> [] not md5("") = d41d8cd98f00
nonexistent tree -> []
all six spellings -> 213b0077e930 (plain, trailing slash, /./, /src/.., symlink, relative .)
And in the module itself, manifest() returns None rather than a short manifest when a digest fails (test/pgc_fingerprint.py:149). That is the distinction that matters and it is the one I would most expect a port to lose: skipping the unreadable file would produce a well-formed hash over fewer files, which is worse than the defect #909 fixed, not better.
The absence arms fire against the real files
This is the shape I said I would look at hardest, because a static assertion of ABSENCE is the one most likely to be unable to fail. I injected into the real files rather than fixtures:
real pgc_cluster.py gains `hashlib.md5`
427 passed + 1 failed
FAIL the pytest helper keeps no private fingerprint implementation: got [1] want [0]
real pgc_fingerprint.py gains `from pgc_cluster import ...`
400 passed + 28 failed, including
FAIL the module imports nothing from the pytest tree: got [1] want [0]
The first is surgical — exactly one arm. The second's blast radius is 28 because that import breaks the module outright under the system interpreter, which is precisely why the arm exists.
I nearly filed a false blocker, and the correction is worth more than the finding would have been
I probed "what if python3 is absent" and measured the fingerprint as the string -- python3 not found: ..., then traced the consequence: both sides carrying that string compare EQUAL, so the verdict would be fresh — a false green on every suite, strictly worse than anything in this family.
It was my instrument. I captured the subshell with 2>&1, and that warning goes to stderr (lib.sh:725). Measured properly:
python3 absent -> value [] verdict unknown -> UNVERIFIED correct
stamp written with python3, read without -> unknown correct
warning appears on stderr, once
What made me re-check was the source reading >&2 while my measurement said otherwise. When the instrument and the code disagree, suspect the instrument — I have spent today saying that to other people.
One residual, not blocking
_manifest_files skips an unreadable directory with continue (pgc_fingerprint.py:109), producing a well-formed hash over fewer files. That is faithful to find, which prints nothing for a directory it cannot read, so it is equivalence-preserving and pre-existing rather than introduced here — and equivalence is this PR's contract, so I would not change the behaviour.
But the module's thesis is fail-closed, and this is the one place it is fail-open by inheritance. The per-file path has a comment explaining why None beats a short manifest; the per-directory path has a comment explaining what find does. A sentence saying the asymmetry is deliberate would stop the next reader treating it as an oversight. I could not exercise it — the tests run as root here and root ignores the mode bits — so this is a reading, not a measurement, and I have labelled it as such.
Verified
harness_selftest 428 passed + 0 failed + 0 unrunnable
CI 12/12, CLEAN
The direction is right and your measurement settled it against your own prior: one interpreter start at 26 ms beats 64 forks at 239 ms, and 124 s of gate time becomes 13 s. Two implementations of one idea produced four defects in a day and not one was found by whoever wrote that copy; one implementation with two observers is the shape that has been earning its keep all day.
Closes #907.
test/lib.shandtest/pytest/pgc_cluster.pyeach carried their own answer to"what was this binary built from". In one day the pair produced four defects, two
in each copy, and not one was found by whoever wrote that copy:
objstore/*.cnever walkedsrc/module.candobjstore/module.cwere interchangeablexargs -0 cat | md5sum— no paths, no boundariesMakefileomittedThe Python docstring asserted "the same input set as
pgc_source_fingerprintintest/lib.sh" through all four. It was false when written and stayed falsethrough two rounds of fixing.
Python rather than shell, which is the opposite of what #907 proposed
jd's constraint decided the direction: the single implementation belongs in the
more portable language. bash is largely a GNU thing; Python is on FreeBSD and
Windows where bash is not.
lib.shalready requires bash, so calling a moreportable interpreter from it cannot cost portability.
I argued against this direction —
lib.shinvokespython3zero times, so itescalates from "53 suites need it" to "every suite needs it at gate time". True,
and not a cost, for the reason above. Then I measured it expecting a subprocess
penalty:
9x faster, because the shell paid 64 forks per call. I had it backwards in
both dimensions.
Stdlib only, and it runs on the system interpreter:
test/pytest/README.mdrecords that the interpreter is
EXTERNALLY-MANAGEDand pytest runs from a venv,and a freshness gate that needed the pytest test dependencies would make every
bash suite unrunnable until somebody installed pytest.
A fifth defect, which unifying them found
sort -zorders by locale collation and nothing in the harness pins a locale,so one tree fingerprinted two ways depending on the machine:
en_US.UTF-8is a common desktop default, so a developer could stamp a tree andhave CI read it back and call the binary stale — a false FATAL arriving from the
environment rather than the source. The module sorts bytes, which is what
LC_ALL=Cproduced and what every stamp already on disk was written with, sonothing is invalidated. Arms in both harnesses.
Equivalence established, not asserted
A differential run of the module against the shell it replaces, over trees built
to break the ways this pair has actually broken — manifest and fingerprint
compared on each:
Two are subtle enough to name.
find -type ftests the link, so a symlinkedsource is not in the shell's manifest, while
pathlib.is_file()follows it andwould have added one — the module excludes symlinks explicitly. And
finddoesnot descend a symlinked directory, which is why the module canonicalises the root
before discovering build dirs.
Two arms had to change mechanism, and why that is not weakening them
The failed-digest arms stubbed
md5sumonPATH. The digest ishashlibnow,which no
PATHcan reach, so the stub would have left both arms green whiletesting nothing — the exact shape this corpus exists to refuse.
A real read failure needs a real reader who is denied, and root is denied nothing:
So the tree is built outside any mode-0700 directory and read by a second user,
with a premise asserting that reader agrees with a privileged one while nothing
is denied — otherwise the arm measures the user switch rather than the read
failure. Where no non-root user exists it records
expect.cannot_runrather thanpassing.
The arm that would catch this issue recurring
selftest 380's static guards follow the fingerprint to its new file, plus three
new arms: neither caller may keep a private implementation, and the module may
import nothing from
test/pytest/.A static assertion of absence is the shape that most often cannot fail, so
each was proved against the real files rather than only against fixtures — a
fixture proves the pattern matches something, not that the arm aimed at the real
file would fire (@jdatcmd made this a condition, and he was right to):
Note on ordering
Based on
mainata9c40b1. #905 and #906 are ahead of it in the merge queue andboth touch
TESTS.md, so this will need a recount-not-pick rebase once they land.I will do that rather than resolve it in anyone else's branch.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a